Skip to content

preserve responses text format and resolve prompt cache retention#313

Closed
ndycode wants to merge 6 commits intofeat/openai-parity-pr4from
feat/openai-parity-pr5
Closed

preserve responses text format and resolve prompt cache retention#313
ndycode wants to merge 6 commits intofeat/openai-parity-pr4from
feat/openai-parity-pr5

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 22, 2026

Summary

  • preserve text.format through transformer defaults and fast-session tuning
  • resolve prompt_cache_retention from the request, provider options, or model config
  • add focused regression coverage for the request-contract fields

Stack

  • base: #312

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr fixes two quiet but impactful request-contract bugs: text.format was being silently dropped on transformer re-assignment (now preserved via object spread in both the normal and fast-session paths), and prompt_cache_retention had no resolution cascade (now resolved body → providerOptions.openaimodelConfig, consistent with how textVerbosity and include work). it also wires the codex-max short alias into the model map and tightens the PromptCacheRetention type from bare string to a branded open union for better autocomplete.

  • resolvePromptCacheRetention follows the same three-tier priority pattern already established by resolveTextVerbosity and resolveInclude — no concurrency risk, pure function.
  • text.format preservation is correct in both the normal path (body.text = { ...body.text, verbosity: ... }) and the fast-session path (body.text = { ...body.text, verbosity: "low" }).
  • no windows filesystem or token safety risks introduced — changes are in-memory request transforms only.
  • two vitest coverage gaps remain: the new codex-max alias is not asserted in model-map.test.ts, and the fast-session + text.format combination has no dedicated test.

Confidence Score: 5/5

  • safe to merge — logic is correct, no runtime risk, two coverage gaps are non-blocking follow-ups.
  • the core fixes (format preservation and cache-retention cascade) are correct, follow established patterns in the codebase, and are well-tested for the primary paths. the two p2 items (missing alias test, missing fast-session+format test) are coverage improvements, not bugs. no concurrency, windows filesystem, or token safety risks introduced.
  • test/model-map.test.ts and test/request-transformer.test.ts could use the two additional coverage cases called out in comments, but neither blocks merge.

Important Files Changed

Filename Overview
lib/request/request-transformer.ts adds resolvePromptCacheRetention (body → providerOptions → modelConfig priority chain) and fixes text.format preservation via object spread — logic is correct in both the normal and fast-session paths.
lib/types.ts introduces the PromptCacheRetention branded open-string union and wires it into ConfigOptions and RequestBody — well-typed, autocomplete-friendly pattern.
lib/request/helpers/model-map.ts adds addReasoningAliases("codex-max", "gpt-5.1-codex-max") for the short alias — correct placement, but no corresponding coverage in model-map.test.ts.
test/request-transformer.test.ts good coverage of the three-tier prompt_cache_retention priority chain and text.format preservation on the normal path; fast-session + text.format combination is unexercised.
test/public-api-contract.test.ts positional vs named overload contract now asserts both prompt_cache_retention and text.format are preserved — solid regression guard.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[incoming RequestBody] --> B{body.prompt_cache_retention set?}
    B -- yes --> E[use body value]
    B -- no --> C{providerOptions.openai\n.promptCacheRetention set?}
    C -- yes --> E
    C -- no --> D{modelConfig\n.promptCacheRetention set?}
    D -- yes --> E
    D -- no --> F[omit field — no-op]
    E --> G[assign body.prompt_cache_retention]

    A --> H{body.text.format set?}
    H -- yes --> I[spread: body.text = ...\nbody.text + verbosity override]
    H -- no --> I
    I --> J{fastSession?}
    J -- yes --> K[spread: body.text = ...\nbody.text + verbosity = low\nformat preserved]
    J -- no --> L[body.text.format preserved]
    K --> L
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/model-map.test.ts
Line: 14-19

Comment:
**missing vitest coverage for new `codex-max` alias**

`addReasoningAliases("codex-max", "gpt-5.1-codex-max")` is added in model-map.ts but the `MODEL_MAP` snapshot test never asserts that `codex-max`, `codex-max-high`, or `codex-max-xhigh` resolve to `"gpt-5.1-codex-max"`. if the alias registration order or canonical target ever regresses, nothing catches it.

```suggestion
	it("keeps codex families canonical", () => {
		expect(MODEL_MAP["gpt-5-codex"]).toBe("gpt-5-codex");
		expect(MODEL_MAP["gpt-5.3-codex-spark-high"]).toBe("gpt-5-codex");
		expect(MODEL_MAP["gpt-5.1-codex-max-xhigh"]).toBe("gpt-5.1-codex-max");
		expect(MODEL_MAP["codex-max"]).toBe("gpt-5.1-codex-max");
		expect(MODEL_MAP["codex-max-high"]).toBe("gpt-5.1-codex-max");
		expect(MODEL_MAP["codex-mini-latest"]).toBe("gpt-5.1-codex-mini");
	});
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/request-transformer.test.ts
Line: 684-706

Comment:
**missing fast-session + `text.format` coverage**

the existing `preserves text.format` test exercises the normal path. the fast-session branch (`shouldApplyFastSessionTuning = true`) also does `body.text = { ...body.text, verbosity: "low" }`, but no test passes `text.format` through that branch to confirm format survives there too. without it, a future change to the fast-session block could silently clobber `text.format`.

add a sibling test that calls `transformRequestBody` with the `isFastSession` flag set to `true` and a non-null `text.format`, then asserts `result.text?.format` equals the input value and `result.text?.verbosity === "low"`.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (5): Last reviewed commit: "Merge main into feat/openai-parity-pr5" | Re-trigger Greptile

@chatgpt-codex-connector
Copy link

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

📝 Walkthrough

Walkthrough

this change adds prompt cache retention configuration support with a three-tier resolution strategy: direct body assignment, provider-specific options, or model config defaults. the transformer integrates this into the existing request body transformation pipeline without altering core control flow.

Changes

Cohort / File(s) Summary
Type System & Configuration
lib/types.ts
Added PromptCacheRetention type with literal values ("5m" | "1h" | "24h" | "7d") plus extensibility. Updated ConfigOptions and RequestBody interfaces to include/upgrade prompt_cache_retention typing.
Request Transformation Logic
lib/request/request-transformer.ts
Added resolvePromptCacheRetention() function that checks body field, then providerOptions.openai.promptCacheRetention, then modelConfig.promptCacheRetention. Integrated into transformRequestBody to conditionally assign resolved value after verbosity handling.
Test Coverage
test/public-api-contract.test.ts, test/request-transformer.test.ts
Enhanced public API contract test with prompt_cache_retention: "24h" and JSON schema format. Added request-transformer.test.ts cases validating resolution priority across body, provider options, and model config sources.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Notable gaps & concerns

  • no regression test for invalid values: type system accepts string & {} for extensibility, but tests don't validate behavior with unexpected values like "99h" or malformed inputs
  • resolution order not explicitly sequenced: lib/request/request-transformer.ts:~line sets body.text.verbosity before prompt_cache_retention, but no test validates they resolve in documented order or interact correctly
  • provider options nesting: deep path check at body.providerOptions.openai.promptCacheRetention assumes structure exists; tests cover happy path but not malformed/missing intermediate objects
🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning title does not follow conventional commits format (missing type(scope): prefix) and reads as a summary statement rather than an imperative command. rewrite title as feat: preserve responses text format and resolve prompt cache retention or similar, ensuring it matches <type(scope): summary> format with lowercase imperative verb.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed PR description is detailed and follows template structure with summary, what changed, validation checklist, and risk assessment, though docs/governance and rollback sections are incomplete.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/openai-parity-pr5
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch feat/openai-parity-pr5

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode ndycode changed the title type responses text format and prompt cache retention preserve responses text format and resolve prompt cache retention Mar 22, 2026
@ndycode ndycode added the needs-human-review Flagged by automated PR quality screening; maintainer review required. label Mar 22, 2026
coderabbitai[bot]
coderabbitai bot previously requested changes Mar 22, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/public-api-contract.test.ts`:
- Around line 119-133: The contract test in test/public-api-contract.test.ts
currently only compares positional vs named equality, which can miss dropped
fields; update the test to assert explicitly that the transformed outputs
include prompt_cache_retention and the full text.format object (including
type/name/schema/strict and compat_response schema properties) and that their
values match the input; locate the variables used for the transformed results
(e.g., positionalResult/namedResult or whatever the test uses to hold the two
outputs) and add assertions that positionalResult.prompt_cache_retention ===
input.prompt_cache_retention and namedResult.prompt_cache_retention ===
input.prompt_cache_retention, plus deep-equality assertions for
positionalResult.text.format and namedResult.text.format against the original
text.format object (including compat_response schema), so any accidental
stripping is caught.

In `@test/request-transformer.test.ts`:
- Around line 1286-1297: Add a regression test to verify model-level and
named-params overload inheritance of promptCacheRetention: extend
test/request-transformer.test.ts with cases that call transformRequestBody using
a UserConfig where models['some-model'].options.promptCacheRetention is set and
assert result.prompt_cache_retention matches that value, and also call the
named-params overload of transformRequestBody (passing model and input as
separate args) with the same model-level config to ensure the resolver behaves
identically; reference transformRequestBody, UserConfig,
models[...].options.promptCacheRetention, RequestBody and the named-params
invocation form to locate where to add these deterministic vitest assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c1964911-d1ca-477b-a78e-7597cc248099

📥 Commits

Reviewing files that changed from the base of the PR and between 71d44c7 and 21df813.

📒 Files selected for processing (4)
  • lib/request/request-transformer.ts
  • lib/types.ts
  • test/public-api-contract.test.ts
  • test/request-transformer.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

  • lib/request/request-transformer.ts
  • lib/types.ts
test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

  • test/public-api-contract.test.ts
  • test/request-transformer.test.ts
🔇 Additional comments (3)
lib/request/request-transformer.ts (1)

201-211: good precedence and safe assignment for prompt cache retention

this is clean. lib/request/request-transformer.ts:201 (line 201) and lib/request/request-transformer.ts:920 (line 920) implement the expected resolution order and only write when defined, which preserves host intent and avoids clobbering unset requests. coverage is present in vitest at test/request-transformer.test.ts:637 and test/request-transformer.test.ts:1286.

Also applies to: 920-923

test/request-transformer.test.ts (1)

637-649: nice precedence coverage for body vs provider options

these cases at test/request-transformer.test.ts:637 (line 637) and test/request-transformer.test.ts:651 (line 651) are strong and deterministic, and they directly lock the new precedence behavior.

Also applies to: 651-664

lib/types.ts (1)

27-37: type contract update is coherent

the new alias at lib/types.ts:31 (line 31) and its usage in lib/types.ts:27 and lib/types.ts:142 are consistent and align the config/request contracts without widening to any.

Also applies to: 142-142

@ndycode
Copy link
Owner Author

ndycode commented Mar 22, 2026

@coderabbitai review
@greptile @cubic review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 22, 2026

@ndycode sure, kicking off the review now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@ndycode ndycode force-pushed the feat/openai-parity-pr4 branch from 1ad9a65 to b7fef3e Compare March 22, 2026 13:43
@ndycode ndycode removed the needs-human-review Flagged by automated PR quality screening; maintainer review required. label Mar 22, 2026
@ndycode ndycode dismissed coderabbitai[bot]’s stale review March 22, 2026 15:50

All review threads are resolved and later commits addressed this stale automated change request.

@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

Closing because this work is now included in main via #318 and #319.

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants